Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API migration guide. #1133

Merged
merged 31 commits into from
May 22, 2024
Merged

API migration guide. #1133

merged 31 commits into from
May 22, 2024

Conversation

wks
Copy link
Collaborator

@wks wks commented May 6, 2024

An API Migration Guide section is added to the porting guide.

A symbolic link is created as mmtk-core/API-MIGRATION.md which can be easily discovered from the root of the repository.

An *API Migration Guide* section is added to the porting guide.

A symbolic link is created as `mmtk-core/API-MIGRATION.md` which can be
easily discovered from the root of the repository.
@wks wks requested a review from qinsoon May 6, 2024 09:43
Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some changes to the PR:

  1. Add a CI check to make sure migration guide is updated if we have API changes.
  2. Remove the symbolic link (symbolic links don't work for OS like windows). Instead, refer to the migration guide in README.
  3. Move migration guide to a folder so we may extend it and split it based on versions.

I proposed using a more concise and more organised format for the migration guide.

## 0.25.0


### `ObjectReference` is no longer nullable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would expect most people to just bump the MMTk version and attempt to fix on their own. When they struggle to do that, the migration guide needs to help them locate the issues they are facing. We should expect people to glance through the list and find related information for them. Thus I suggest using a more concise and more organised format.

I propose something like below:

### `ObjectReference` is no longer nullable: https://github.com/mmtk/mmtk-core/pull/1064
* Affected: everyone
* Expected changes:
  * `ObjectReference` cannot refer to a null reference.
  * Use `Option<ObjectReference>` (Rust) or `mmtk::util::api_util::NullableObjectReference` (for native API).
* Example migration: https://github.com/mmtk/mmtk-openjdk/pull/265

### Write barriers use `Option<ObjectReference>` for `target`: https://github.com/mmtk/mmtk-core/pull/1130
* Affected: bindings using generational plans
* Expected changes: 
  * Bindings should properly pass `target: Option<ObjectRefernece>` for `object_reference_write_pre/post`.
  * `object_reference_write` is marked as deprecated and will be redesigned. Use `object_reference_write_pre/post` instead, for now.
* Example: https://github.com/mmtk/mmtk-openjdk/pull/273

### Instance methods of `ObjectReference` changed: https://github.com/mmtk/mmtk-core/pull/1122
* Affected: everyone
* Expected changes:
  * Some methods in `ObjectReference` expect a type parameter `<VM: VMBinding>`.
* Example: https://github.com/mmtk/mmtk-openjdk/pull/

### The GC controller (a.k.a. coordinator) is removed: https://github.com/mmtk/mmtk-core/pull/1067
* Affected: everyone
* Expected changes:
  * Code related with the GC controller is removed, and the bindings no longer need to spawn and run a GC controller.
* Example: https://github.com/mmtk/mmtk-openjdk/pull/268

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the binding developers find an API changed, they want to know why this change is made, and more importantly, how they should change their existing usage. The problem with this format is that it is overly concise. For example, if we mention "Edge::load() now returns Option<ObjectReference>", the reader will think "What should it return after this change?" That's where the long explanation is needed.

But you are right. Most people expect to quickly locate the related information. I think we can add the "expected changes" list to the beginning of each item as a concise TL;DR-style hint, while the reader can continue reading if they find relevant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer a list of changes in the migration guide, such as https://getbootstrap.com/docs/5.0/migration/. Besides, the links to PRs should include all the information which people can continue reading to know more details.

However, we can maintain a list with optional links to detailed description for a PR, like https://v3-migration.vuejs.org/breaking-changes/. e.g.

### `ObjectReference` is no longer nullable: https://github.com/mmtk/mmtk-core/pull/1064
* Affected: everyone
* Details: See here (this is optional)
* Expected changes:
  * `ObjectReference` cannot refer to a null reference.
  * Use `Option<ObjectReference>` (Rust) or `mmtk::util::api_util::NullableObjectReference` (for native API).
* Example: https://github.com/mmtk/mmtk-openjdk/pull/265

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, if we mention "Edge::load() now returns Option<ObjectReference>", the reader will think "What should it return after this change?" That's where the long explanation is needed.

We state that ObjectReference cannot be null any more, ad Option<ObjectReference> should be used. I think this should be enough for users to know how to deal with a return value with Option<ObjectReference>. Plus, we have updated comments for the function Edge::load() for details.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made another version in the bullet points style: https://github.com/wks/mmtk-core/blob/0db7a487a28d54a22949a81033878d862fa11081/docs/userguide/src/migration/prefix.md

It does look clearer at first glance, but I feel it is not right. It looks good as a summary of API changes, but that's probably not what VM binding developers need. When a binding developer pulls the mmtk-core repository, the binding will fail to compile, and the compiler and the IDE will highlight functions that have problems. That will be the same set of functions we listed in the bullet points version. I don't expect the VM binding developers to read through the bullet points in the Migration Guide because the compile errors will tell them what needs to be changed. If the change is obvious, the binding developer will just make the change and fix the compilation error.

The problem is, what if the binding developer finds one of the functions no longer compiles, and but does not know how to update the old code to adapt to the change? They will eventually find the answer by reading the API docs of affected functions. But it is easier if they can go to the Migration Guide, and find detailed descriptions about the old behavior and the new behavior. The Migration Guide should tell the reader what to do if the binding wants to do this or that.

I find the migration guide of the sysinfo crate quite helpful, especially the following part:

A new System::refresh_memory_specifics method and a new MemoryRefreshKind type were added, allowing you to control whether you want both RAM and SWAP memories to be updated or only one of the two. This change was needed because getting SWAP information on Windows is very slow.

MMTk used RefreshKind::new().with_memory() in get_system_total_memory. After bumping the version, it no longer compiles. I figured out how to make the change by reading that paragraph which told me I should switch to MemoryRefreshKind and also decide if I want the RAM size or the swap size. Because MMTk wants the physical memory size, I changed it to `MemoryRefreshKind::new().with_ram().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is, what if the binding developer finds one of the functions no longer compiles, and but does not know how to update the old code to adapt to the change?

Exactly. I think most developers won't go straight to the migration guide, but instead they just pull the new version and try to fix compilation errors. They will turn to the migration guide when they find that they cannot fix some issues themselves. Hopefully the migration guide can help them locate where the compilation error is from, and provide them a link to an example -- that should be enough for most cases. So I suggest the migration guide to be concise and organized so that they can easily locate the issue they are searching for, and they can find the solution with an example, or with detailed migration description and the related PR.

Copy link
Collaborator Author

@wks wks May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a third version https://github.com/wks/mmtk-core/blob/feature/api-change-log3/docs/userguide/src/migration/prefix.md

It still has the bullet points style, but is not as concise as the second version, and contains more explanation. For the convenience of navigation, it now organises the changes in a hierarchy of (1) type, trait or module, and then (2) method. It is probably only needed for big changes like removing ObjectReference.

@wks
Copy link
Collaborator Author

wks commented May 7, 2024

Add a CI check to make sure migration guide is updated if we have API changes.

This should give a warning instead of failing immediately. Some API changes add new methods (such as adding new APIs), while others are source compatible (such as adding a type parameter <T> which can be automatically inferred). Those changes don't need the binding to be changed.

@qinsoon
Copy link
Member

qinsoon commented May 7, 2024

Add a CI check to make sure migration guide is updated if we have API changes.

This should give a warning instead of failing immediately. Some API changes add new methods (such as adding new APIs), while others are source compatible (such as adding a type parameter <T> which can be automatically inferred). Those changes don't need the binding to be changed.

Sounds good. I added the new check to the ignore list of merge-check.

Edit: I thought I pushed this 72b236e.

@wks wks mentioned this pull request May 13, 2024
@wks
Copy link
Collaborator Author

wks commented May 13, 2024

I changed the style so that the first two levels of the lists form an outline of the changes, and the third level of the list contains more details and explanations.

I also use JavaScript to let the reader collapse and expand the third level of the lists. When collapsed, the list will look like a terse outline, but the readers can still expand individual items if they desire. This functionality is only available in the rendered HTML version.

I also moved the template from the comments into a dedicated page.

@qinsoon
Copy link
Member

qinsoon commented May 14, 2024

This is what the current form looks like for one API breaking PR, when we hide 'collapsible' sections.

api-migration

Users need to be able to locate the information they need by looking at the list. If we have 10 entries like above in the list, the users won't be able to easily find the entry that they are looking for. You can just try this yourself: copy and paste that one entry 10 times, and see if you can easily locate the title for each entry. I find it hard. If users cannot even find the titles easily, they will have a hard time to find the right entry for their needs.

I suggest something like this.

### `ObjectReference` is no longer nullable

**TL;DR** `ObjectReference` can no longer represent a NULL reference.  Some methods of
`ObjectReferences` and write barrier functions are changed.  VM bindings need to re-implement
methods of the `Edge`, `ObjectModel` and `ReferenceGlue` traits.

COLLAPSIBLE START
All the detailed description goes here
COLLAPSIBLE END

We can use something like https://crates.io/crates/mdbook-admonish for collapsible blocks.

@wks
Copy link
Collaborator Author

wks commented May 14, 2024

Users need to be able to locate the information they need by looking at the list.

In my opinion, the information the VM binding developers need the most is the detailed guide to change a particular type/method when they don't know how to fix that compilation error. So there needs to be an option to expand everything (or just the type and method names) and let the developer search for the method name they want to locate.

But it is nice to have an option to hide everything other than the TL;DR part just in case someone wants to skim through all changes and see if it is worth upgrading to a particular version. I'll make the change.

@wks
Copy link
Collaborator Author

wks commented May 14, 2024

We can use something like https://crates.io/crates/mdbook-admonish for collapsible blocks.

Thank you. That's something I was looking for. I even tried to replicate what mdbook-admonish does. But one problem with that is that Admonish is intended to draw attention. But I want the folded details to be as quiet as possible so that the reader can focus on things that are not folded. I think we can use Admonish for the TL;DR part, instead.

@qinsoon
Copy link
Member

qinsoon commented May 14, 2024

So there needs to be an option to expand everything (or just the type and method names) and let the developer search for the method name they want to locate.

In your implementation, you use the <details> tag so those collapsible sections are rendered in DOM. You can do in-page search from browsers to find text in collapsed sections without the need to expanding them first. I think that is nice.

@qinsoon
Copy link
Member

qinsoon commented May 14, 2024

I think we can use Admonish for the TL;DR part, instead.

If you mean use admonish to highlight TLDR, that's fine. But don't make TLDR collapsible.

@wks
Copy link
Collaborator Author

wks commented May 14, 2024

Now we have the option to fold everything except the TL;DR.

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format-wise, I think it is good now. I have some questions about the content.

Also there is one question we need to answer: do we expect external contributors to write the migration guide when they introduce a API breaking change? We could make it optional, and require maintainers help write the migration guide instead.

docs/userguide/src/migration/prefix.md Outdated Show resolved Hide resolved
docs/userguide/src/migration/template.md Show resolved Hide resolved
docs/userguide/src/migration/template.md Show resolved Hide resolved
docs/userguide/src/migration/template.md Outdated Show resolved Hide resolved
+ Note: The `Edge` trait does not have a method for storing NULL to a slot. The VM
binding needs to implement its own method to store NULL to a slot.

Miscellaneous changes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a separate title here? The things here seem to belong to the 'API changes' part.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because strictly speaking, the API is not changed w.r.t. get_finalized_object and get_forwarded_object. They returned Option<ObjectReference> before, and they still return Option<ObjectReference>. However, it is worth mentioning because the binding may expose them to C/C++, and that will fail to compile if the extern "C" functions return ObjectReference which was previously nullable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. You mean the API is compliant but the semantic is changed. Are there any changes other than semantic changes that should be put as 'miscellaneous changes'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semantics is not changed, either. get_finalized_object used to return Option<ObjectReference> which meant "there is no finalized object". It still returns Option<ObjectReference> and still means the same. The problem is how the VM binding exposes it to C++. MMTk does not provide an official C/C++ API, so the exposed extern "C" wrapper is not controlled by MMTk-core. I mentioned that because many bindings do that, i.e. they use ObjectReference::NULL to encode None, which is no longer possible.

Strictly speaking, the root cause of that is that ObjectReference::NULL became non-nullable. The extern "C" wrappers of get_finalized_object and get_forwarded_object are just two possible use cases of ObjectReference::NULL. But since we also introduced a NullableObjectReference, we mention that in "miscellaneous changes".

If there are other things that have non-obvious but better way to do, we can mention them in "miscellaneous changes", too. And of course there are other use cases that I can't foresee now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to "Not API change, but worth noting". The purpose should be clear now.

@@ -36,6 +36,8 @@
- [Performance Tuning](portingguide/perf_tuning/prefix.md)
- [Link Time Optimization](portingguide/perf_tuning/lto.md)
- [Optimizing Allocation](portingguide/perf_tuning/alloc.md)
- [API Migration Guide](migration/prefix.md)
- [Template (for mmtk-core developers)](migration/template.md)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need to list the template in mdbook? For people who need to edit the migration guide, they will view the template as the raw markdown file rather than the rendered HTML version. For people who do not edit the migration guide, they do not need to see this template in the migration guide at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer them rendered so that editors know what to expect when looking at the rendered version. But I agree that ordinary readers may not need to read it. I'll see if we can make it a hidden section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I use mdbook-hide to hide the chapter, but only in the published version. It is shown if we build the mdbook locally.

docs/userguide/book.toml Outdated Show resolved Hide resolved
@wks
Copy link
Collaborator Author

wks commented May 22, 2024

I simply removed the sub-title "VM bindings should reimplement ..." and merged the items into "API changes".

@qinsoon
Copy link
Member

qinsoon commented May 22, 2024

Broken link found:

### Errors in docs/userguide/src/portingguide/howto/nogc.md

* [404] [https://docs.mmtk.io/api/mmtk/vm/edge_shape/trait.MemorySlice.html](https://docs.mmtk.io/api/mmtk/vm/edge_shape/trait.MemorySlice.html) | Failed: Network error: Not Found

It should be from the last PR that renamed slot.

@qinsoon
Copy link
Member

qinsoon commented May 22, 2024

I simply removed the sub-title "VM bindings should reimplement ..." and merged the items into "API changes".

The template is not updated.

@wks
Copy link
Collaborator Author

wks commented May 22, 2024

I have just updated the template.

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wks wks added this pull request to the merge queue May 22, 2024
Merged via the queue into mmtk:master with commit 170630f May 22, 2024
23 of 24 checks passed
@wks wks deleted the feature/api-change-log branch May 22, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants